-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGFIX beta] Ensures Ember.A is a constructor #17238
[BUGFIX beta] Ensures Ember.A is a constructor #17238
Conversation
cc @rwjblue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we intended new Ember.A()
(I’ve generally always used it without new
), but it seems fine to fix if recent changes broke it.
Can you also add a test?
Lets add a private API deprecation ( |
9181605
to
b6cd574
Compare
Updated, and added PR to the deprecation app |
FWIW, there is a bit of usage of https://emberobserver.com/code-search?codeQuery=new%20A( |
b6cd574
to
488195d
Compare
{ | ||
id: 'array.new-array-wrapper', | ||
until: '3.9.0', | ||
url: 'https://emberjs.com/deprecations/v3.x#toc_new-array-wrapper', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be wrong (the url doesn't link directly), when I find the right link it looks like:
https://deprecations-app-prod.herokuapp.com/deprecations/v3.x/#toc_array-new-array-wrapper
Can you confirm which is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct! Updated
The recent change to transpilation has made `new A()` break, because A is an arrow function and arrow functions cannot be constructors. This is a pretty common pattern in Ember apps and likely constitutes a breaking change, so this PR fixes it by changing the arrow function to a normal function, and deprecates this behavior.
488195d
to
5a565d5
Compare
|
||
['@test new Ember.A'](assert) { | ||
expectDeprecation(() => { | ||
assert.deepEqual(new A([1, 2]), [1, 2], 'array values were not be modified'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, woah this was never an intended feature...
The recent change to transpilation has made
new A()
break, becauseA is an arrow function and arrow functions cannot be constructors. This
is a pretty common pattern in Ember apps and likely constitutes a
breaking change, so this PR fixes it by changing the arrow function to
a normal function.